Add safe request timeout for running actions manager #743

zbirenbaum · 2024-03-10T01:03:21Z

Description

Backport of running actions manager timeout changes which checks the timeout for an action and defaults to a maximum if not set.

Type of change

New feature (non-breaking change which adds functionality)

Checklist

bazel test //... passes locally
PR is contained in a single commit, using git amend see some docs

This change is

allada

Reviewable status: 0 of 1 LGTMs obtained

a discussion (no related file):
lgtm, but we need a test.

adam-singer

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: 0 of 1 LGTMs obtained

zbirenbaum

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: pre-commit-checks

a discussion (no related file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

lgtm, but we need a test.

Test has been added

adam-singer

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: 1 of 1 LGTMs obtained, and pending CI: pre-commit-checks

adam-singer

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: 1 of 1 LGTMs obtained, and pending CI: windows-2022 / stable

allada

Reviewable status: 1 of 1 LGTMs obtained, and pending CI: windows-2022 / stable

nativelink-worker/tests/running_actions_manager_test.rs line 2660 at r4 (raw file):

    #[tokio::test]
    async fn running_actions_manager_respects_action_timeout() -> Result<(), Box<dyn std::error::Error>> {
        #[cfg(target_family = "unix")]

nit: This should probably be removed.

nativelink-worker/tests/running_actions_manager_test.rs line 2698 at r4 (raw file):

        )
        .await?;
        const TASK_TIMEOUT: Duration = Duration::from_secs(1);

Tests should not use wall-time. In this case, you probably want to use RunningActionsManagerImpl::new_with_timeout or something to give test control of the timeout function, then use some signaling to facilitate this test.

zbirenbaum

Reviewable status: 1 of 1 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / ubuntu-22.04, Local / ubuntu-22.04, Remote / large-ubuntu-22.04, Vercel, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), pre-commit-checks, publish-image, ubuntu-20.04 / stable, ubuntu-22.04, vale, zig-cc ubuntu-20.04, zig-cc ubuntu-22.04

nativelink-worker/tests/running_actions_manager_test.rs line 2698 at r4 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

Tests should not use wall-time. In this case, you probably want to use RunningActionsManagerImpl::new_with_timeout or something to give test control of the timeout function, then use some signaling to facilitate this test.

I think the easiest way to do this without actually waiting is to set both the timeout and max_timeout to 0, at which point the checks for time elapsed will just always return true.

adam-singer

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: 1 of 1 LGTMs obtained, and pending CI: windows-2022 / stable

notion-workspace · 2024-03-26T18:27:03Z

Add safe request timeout for running actions manager

allada

Reviewed 1 of 2 files at r6.
Reviewable status: 1 of 1 LGTMs obtained, and pending CI: windows-2022 / stable, and 3 discussions need to be resolved

nativelink-worker/tests/running_actions_manager_test.rs line 2698 at r4 (raw file):

Previously, zbirenbaum (Zach Birenbaum) wrote…

I think the easiest way to do this without actually waiting is to set both the timeout and max_timeout to 0, at which point the checks for time elapsed will just always return true.

I'm not a huge fan of doing this, it makes future bugs even more difficult to write tests for and people will end up copy-pasta the pattern around.

It should be fairly trivial to hand in a custom sleep function and allow the test to override it here, we do this in quite a few places.

adam-singer

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: 1 of 1 LGTMs obtained, and pending CI: windows-2022 / stable, and 3 discussions need to be resolved

zbirenbaum

Reviewable status: 1 of 1 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Bazel Dev / ubuntu-22.04, Publish image, Vercel, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), pre-commit-checks, ubuntu-20.04 / stable, ubuntu-22.04, and 2 discussions need to be resolved

nativelink-worker/tests/running_actions_manager_test.rs line 2660 at r4 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

nit: This should probably be removed.

Done.

nativelink-worker/tests/running_actions_manager_test.rs line 2698 at r4 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

I'm not a huge fan of doing this, it makes future bugs even more difficult to write tests for and people will end up copy-pasta the pattern around.

It should be fairly trivial to hand in a custom sleep function and allow the test to override it here, we do this in quite a few places.

I've updated it to take a custom sleep function which will immediately return if the correct timeout is passed and stall indefinitely causing the action to complete and the test to fail otherwise.

allada

Reviewed 1 of 2 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: 2 of 1 LGTMs obtained, and pending CI: windows-2022 / stable, and 2 discussions need to be resolved

nativelink-worker/tests/running_actions_manager_test.rs line 2944 at r8 (raw file):

        fs::create_dir_all(&root_work_directory).await?;

        // ignore the sleep and immediately timeout

nit: Caps and period.

nativelink-worker/tests/running_actions_manager_test.rs line 2973 at r8 (raw file):

                // otherwise return pending and fail the test.
                sleep_fn: |duration| {
                    if duration.as_secs() == ACTION_TIMEOUT as u64 {

nit: For readability, assert that duration is ACTION_TIMEOUT and then return ready.

adam-singer

Reviewed all commit messages.
Reviewable status: 2 of 1 LGTMs obtained, and pending CI: windows-2022 / stable, and 2 discussions need to be resolved

zbirenbaum

Reviewable status: 2 of 1 LGTMs obtained, and pending CI: Vercel, pre-commit-checks

nativelink-worker/tests/running_actions_manager_test.rs line 2944 at r8 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

nit: Caps and period.

Done.

nativelink-worker/tests/running_actions_manager_test.rs line 2973 at r8 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

nit: For readability, assert that duration is ACTION_TIMEOUT and then return ready.

Done.

adam-singer

Reviewed 2 of 2 files at r10, all commit messages.
Reviewable status: 2 of 1 LGTMs obtained, and pending CI: windows-2022 / stable

allada

Reviewed 1 of 1 files at r11, all commit messages.
Reviewable status: 2 of 1 LGTMs obtained, and pending CI: integration-tests (20.04), integration-tests (22.04), windows-2022 / stable

zbirenbaum

Reviewed 2 of 2 files at r12, all commit messages.
Reviewable status: 2 of 1 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Local / ubuntu-22.04, Publish image, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, pre-commit-checks, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable, zig-cc ubuntu-20.04, zig-cc ubuntu-22.04

zbirenbaum

Reviewable status: 2 of 1 LGTMs obtained, and pending CI: integration-tests (20.04), integration-tests (22.04), windows-2022 / stable, and 1 discussions need to be resolved

nativelink-worker/tests/running_actions_manager_test.rs line 3259 at r12 (raw file):

        let arguments = vec!["sh".to_string(), "-c".to_string(), "sleep 2".to_string()];
        #[cfg(target_family = "windows")]
        let arguments = vec![

This is what is causing the test to fail. It is returning error code 1 which is incorrect function. I have been reading up on it but can't figure it out so I'm just going to remove to remove this test for windows if that's alright @allada

adam-singer

Reviewed all commit messages.
Reviewable status: 2 of 1 LGTMs obtained, and pending CI: integration-tests (20.04), integration-tests (22.04), windows-2022 / stable, and 1 discussions need to be resolved

allada

Reviewable status: 2 of 1 LGTMs obtained, and pending CI: integration-tests (20.04), integration-tests (22.04), windows-2022 / stable, and 1 discussions need to be resolved

nativelink-worker/tests/running_actions_manager_test.rs line 3259 at r12 (raw file):

Previously, zbirenbaum (Zach Birenbaum) wrote…

This is what is causing the test to fail. It is returning error code 1 which is incorrect function. I have been reading up on it but can't figure it out so I'm just going to remove to remove this test for windows if that's alright @allada

Why not down below set the error code on windows to expect exit code 1?

zbirenbaum

Reviewable status: 2 of 1 LGTMs obtained, and pending CI: integration-tests (20.04), integration-tests (22.04), windows-2022 / stable, and 1 discussions need to be resolved

nativelink-worker/tests/running_actions_manager_test.rs line 3259 at r12 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

Why not down below set the error code on windows to expect exit code 1?

Judging by the docs it seems like exit code 1 corresponds to something else that wouldn't actually be showing the proper behavior here. It's difficult to tell so I guess its possible that's the expected error code but I'm not certain. If not being sure is preferable to removing the test we can do that

allada

Reviewable status: 2 of 1 LGTMs obtained, and pending CI: integration-tests (20.04), integration-tests (22.04), windows-2022 / stable, and 1 discussions need to be resolved

nativelink-worker/tests/running_actions_manager_test.rs line 3259 at r12 (raw file):

Previously, zbirenbaum (Zach Birenbaum) wrote…

Judging by the docs it seems like exit code 1 corresponds to something else that wouldn't actually be showing the proper behavior here. It's difficult to tell so I guess its possible that's the expected error code but I'm not certain. If not being sure is preferable to removing the test we can do that

This test is not actually testing the error code, it's checking that the timeout triggers. The error code is a side-effect. In this case, I would just make a TODO and keep the test for windows.

Changes running actions manager to use the action timeout if set to a nonzero value for deciding when to kill a process, otherwise defaults to its configured max_action_timeout

zbirenbaum

Reviewable status: 2 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Remote / large-ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), ubuntu-20.04 / stable, ubuntu-22.04 / stable, windows-2022 / stable, zig-cc ubuntu-22.04

nativelink-worker/tests/running_actions_manager_test.rs line 3259 at r12 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

This test is not actually testing the error code, it's checking that the timeout triggers. The error code is a side-effect. In this case, I would just make a TODO and keep the test for windows.

Done.

allada requested changes Mar 10, 2024

View reviewed changes

zbirenbaum force-pushed the v0.1-timeout-changes branch from 008ed7d to c345f7a Compare March 10, 2024 23:48

adam-singer approved these changes Mar 11, 2024

View reviewed changes

zbirenbaum force-pushed the v0.1-timeout-changes branch from c345f7a to 0dc8f59 Compare March 11, 2024 20:17

zbirenbaum commented Mar 11, 2024

View reviewed changes

adam-singer approved these changes Mar 11, 2024

View reviewed changes

zbirenbaum requested a review from allada March 11, 2024 20:40

zbirenbaum force-pushed the v0.1-timeout-changes branch from 0dc8f59 to 54afc90 Compare March 11, 2024 22:06

adam-singer approved these changes Mar 11, 2024

View reviewed changes

allada requested changes Mar 17, 2024

View reviewed changes

zbirenbaum force-pushed the v0.1-timeout-changes branch 3 times, most recently from 2992810 to 1fbcf47 Compare March 25, 2024 20:28

zbirenbaum commented Mar 25, 2024

View reviewed changes

adam-singer approved these changes Mar 26, 2024

View reviewed changes

allada force-pushed the main branch 2 times, most recently from a2a06c2 to 09defc6 Compare March 26, 2024 18:19

zbirenbaum force-pushed the v0.1-timeout-changes branch from 1fbcf47 to 6280c45 Compare March 26, 2024 22:14

allada requested changes Mar 27, 2024

View reviewed changes

adam-singer approved these changes Mar 27, 2024

View reviewed changes

zbirenbaum force-pushed the v0.1-timeout-changes branch 4 times, most recently from 10e5ba3 to 00a1250 Compare April 1, 2024 06:57

zbirenbaum commented Apr 1, 2024

View reviewed changes

allada approved these changes Apr 1, 2024

View reviewed changes

adam-singer approved these changes Apr 1, 2024

View reviewed changes

zbirenbaum force-pushed the v0.1-timeout-changes branch from 00a1250 to e00911c Compare April 2, 2024 22:19

zbirenbaum commented Apr 2, 2024

View reviewed changes

zbirenbaum force-pushed the v0.1-timeout-changes branch from e00911c to 3789408 Compare April 2, 2024 22:26

adam-singer approved these changes Apr 2, 2024

View reviewed changes

zbirenbaum force-pushed the v0.1-timeout-changes branch from 3789408 to 66e4479 Compare April 3, 2024 07:53

allada approved these changes Apr 3, 2024

View reviewed changes

zbirenbaum force-pushed the v0.1-timeout-changes branch 2 times, most recently from 58c0060 to 9a81488 Compare April 3, 2024 23:28

zbirenbaum commented Apr 3, 2024

View reviewed changes

zbirenbaum commented Apr 4, 2024

View reviewed changes

adam-singer approved these changes Apr 4, 2024

View reviewed changes

allada reviewed Apr 5, 2024

View reviewed changes

zbirenbaum commented Apr 5, 2024

View reviewed changes

allada reviewed Apr 5, 2024

View reviewed changes

Add safe request timeout for running actions manager

4e06270

Changes running actions manager to use the action timeout if set to a nonzero value for deciding when to kill a process, otherwise defaults to its configured max_action_timeout

zbirenbaum force-pushed the v0.1-timeout-changes branch from 9a81488 to 4e06270 Compare April 8, 2024 20:42

zbirenbaum commented Apr 8, 2024

View reviewed changes

zbirenbaum merged commit 33db963 into TraceMachina:main Apr 8, 2024
26 checks passed

zbirenbaum deleted the v0.1-timeout-changes branch April 8, 2024 21:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add safe request timeout for running actions manager #743

Add safe request timeout for running actions manager #743

zbirenbaum commented Mar 10, 2024 •

edited by allada

Loading

allada left a comment

adam-singer left a comment

zbirenbaum left a comment

adam-singer left a comment

adam-singer left a comment

allada left a comment

zbirenbaum left a comment

adam-singer left a comment

notion-workspace bot commented Mar 26, 2024

allada left a comment

adam-singer left a comment

zbirenbaum left a comment

allada left a comment

adam-singer left a comment

zbirenbaum left a comment

adam-singer left a comment

allada left a comment

zbirenbaum left a comment

zbirenbaum left a comment

adam-singer left a comment

allada left a comment

zbirenbaum left a comment

allada left a comment

zbirenbaum left a comment

Add safe request timeout for running actions manager #743

Add safe request timeout for running actions manager #743

Conversation

zbirenbaum commented Mar 10, 2024 • edited by allada Loading

Description

Type of change

Checklist

allada left a comment

Choose a reason for hiding this comment

adam-singer left a comment

Choose a reason for hiding this comment

zbirenbaum left a comment

Choose a reason for hiding this comment

adam-singer left a comment

Choose a reason for hiding this comment

adam-singer left a comment

Choose a reason for hiding this comment

allada left a comment

Choose a reason for hiding this comment

zbirenbaum left a comment

Choose a reason for hiding this comment

adam-singer left a comment

Choose a reason for hiding this comment

notion-workspace bot commented Mar 26, 2024

allada left a comment

Choose a reason for hiding this comment

adam-singer left a comment

Choose a reason for hiding this comment

zbirenbaum left a comment

Choose a reason for hiding this comment

allada left a comment

Choose a reason for hiding this comment

adam-singer left a comment

Choose a reason for hiding this comment

zbirenbaum left a comment

Choose a reason for hiding this comment

adam-singer left a comment

Choose a reason for hiding this comment

allada left a comment

Choose a reason for hiding this comment

zbirenbaum left a comment

Choose a reason for hiding this comment

zbirenbaum left a comment

Choose a reason for hiding this comment

adam-singer left a comment

Choose a reason for hiding this comment

allada left a comment

Choose a reason for hiding this comment

zbirenbaum left a comment

Choose a reason for hiding this comment

allada left a comment

Choose a reason for hiding this comment

zbirenbaum left a comment

Choose a reason for hiding this comment

zbirenbaum commented Mar 10, 2024 •

edited by allada

Loading